Update constructor TypeToTypeInfoMarshaler() signature to static #2536

EugeneKramer · 2019-06-03T03:33:41Z

Summary

Update constructor TypeToTypeInfoMarshaler() signature to static for C#, VB.Net, C++ to match the ILAsm static signature and constructor description: Provides the static class constructor.

Edit 1:
Updates the signatures of the 4 constructors in System.Runtime.InteropServices.CustomMarshalers Namespace to public static.

Edit 2:
Please note that public static constructors also go against security warning - CA2121: Static constructors should be private

Fixes #2529

Update constructor TypeToTypeInfoMarshaler() signature to static for C#, VB.Net, C++ to match the ILAsm static signature and constructor description - "Provides the static class constructor." Resolves dotnet#2529

MSDN-WhiteKnight · 2019-06-03T05:11:26Z

xml/System.Runtime.InteropServices.CustomMarshalers/TypeToTypeInfoMarshaler.xml

@@ -36,11 +36,11 @@
  </Docs>
  <Members>
    <Member MemberName=".cctor">
-      <MemberSignature Language="C#" Value="public TypeToTypeInfoMarshaler ();" />
+      <MemberSignature Language="C#" Value="public static TypeToTypeInfoMarshaler ();" />


Is it really a public static constructor? I don't have .NET 1.1 (API browser claims that it's a latest version where this member is available), but in modern environment one cannot declare static constructor as public in C#, it generates compiler error CS0515.

Is there even a reason why API Docs should include static constructor? It is not really an "API", in a sence that one cannot use it directly. Moreover, currently the docs do not seem to provide any useful information about it, besides the fact it was removed in .NET 2.0.

You're correct. It's a static constructor for a public class. I've removed the public access modifier for the C# and VB.Net static constructor. C++ does not support static constructors, so I've reverted the addition of the static modifier.

While a static constructor cannot be accessed directly, it is called prior to any first instance/usage of the type, which can lead to unusual exceptions such as TypeInitializationException which explicitly note "What often makes this exception difficult to troubleshoot is that static constructors are not always explicitly defined in source code." As such, the presence of a static constructor should be noted in the documentation.

A deprecated static constructor is listed for each of the 4 classes in the System.Runtime.InteropServices.CustomMarshalers Namespace. A separate page for the static constructors is probably unnecessary, but their presence should be noted as a remark.

Appreciate the feedback!

Thanks for submitting this PR and raising the issue of a static constructor appearing in our doc set, @EugeneKramer. Since method signatures are gathered using reflection, I'm not sure that the changed signature won't be overwritten by the next reflection. But I'm also not sure why a static constructor is documented in the first place -- though I don't recall whether 2.0 introduced a change in the way static constructors were handled from 1.0/1.1.

@dend @joelmartinez, do you have any sense of why this constructor is marked as public and appears in the documentation? My guess would be that this is the only static constructor documented in the managed reference.,

I would think that the ILAsm signature also needs to change, although I no longer have any .NET Framework 1.1 assemblies.

For the moment, I'll mark this PR as blocked.

MSDN also had this constructor:
https://msdn.microsoft.com/en-us/library/03faz537(v=vs.80).aspx?toc=xxx

with the same signature that @EugeneKramer is proposing.

@rpetrusha you can access any version of the assemblies at https://apidrop.visualstudio.com/_git/binaries?path=%2Fdotnet&version=GBmaster

apisof.net also lists this constructor, so I think it's fine to have it if we fix the signature
https://apisof.net/catalog/System.Runtime.InteropServices.CustomMarshalers.TypeToTypeInfoMarshaler..cctor()

I still find this bizarre, but the static constructor is clearly public in .NET Framework 1.1. So correcting the syntax is appropriate. Is the correct syntax likely to be overwritten, @mairaw?

Is the correct syntax likely to be overwritten

Yes ... this needs to be fixed in mdoc's signature formatters. It will reset these next time an update is run.

@rpetrusha - As mentioned earlier, there are public static constructors listed for each of the 4 classes in the System.Runtime.InteropServices.CustomMarshalers Namespace with the same public signature. If this PR is going through, should each of their signatures be updated...or should a separate PR be opened?

System.Runtime.InteropServices.CustomMarshalers Namespace:

Provides the static class constructor.

EnumerableToDispatchMarshaler Constructor

EnumeratorToEnumVariantMarshaler Constructor

ExpandoToDispatchExMarshaler Constructor

TypeToTypeInfoMarshaler Constructor

All were deprecated in .Net 2.0.

@joelmartinez I've opened an internal mdoc bug for this:
Bug 97094: [mdoc] wrong signature for public static constructors in .NET Framework 1.1

Per dotnet#2536 (comment) Removed the public access modifier for the C# and VB.Net static constructor. C++ does not support static constructors, so I've reverted the addition of the static modifier.

rpetrusha

Thanks for correcting the C# and Visual Basic syntax, @EugeneKramer. We also need to add the public access modifier, since the static constructor was public in .NET Framework 1.1.

xml/System.Runtime.InteropServices.CustomMarshalers/TypeToTypeInfoMarshaler.xml

…InfoMarshaler.xml Co-Authored-By: Ron Petrusha <[email protected]>

rpetrusha · 2019-06-04T16:46:42Z

Thanks again for submitting this PR, @EugeneKramer, as well as for making the additional changes. And thanks for your patience as we've tried to get this sorted out. I've approved your PR, and I'll merge once our internal build completes successfully.

rpetrusha · 2019-06-04T16:57:26Z

Sorry, @EugeneKramer. I didn't notice your earlier comment about the three additional constructors. Yes, their syntax should also be modified. If you'd like to make the change, you can do it in either this PR or a separate PR. If you'd prefer, we could also do it in a separate PR.

…c static for VB.NET & C# Update constructor EnumerableToDispatchMarshaler() signature to public static for VB.NET & C# per dotnet#2536

dnfclas · 2019-06-05T03:44:11Z

All CLA requirements met.

…blic static for VB.NET & C# Update constructor EnumeratorToEnumVariantMarshaler() signature to public static for VB.NET & C# per dotnet#2536

… for VB.NET & C# Update constructor ExpandoToDispatchExMarshaler() signature to public static for VB.NET & C# per dotnet#2536

EugeneKramer · 2019-06-05T03:52:55Z

The signatures of the 4 constructors in System.Runtime.InteropServices.CustomMarshalers Namespace have been updated to public static:

rpetrusha · 2019-06-05T16:19:23Z

Thank you so much for making the additional changes, @EugeneKramer! I'll merge your PR now. The changes should be live on docs.microsoft.com in the next day or two.

Update constructor TypeToTypeInfoMarshaler() signature to static

a31dd2a

Update constructor TypeToTypeInfoMarshaler() signature to static for C#, VB.Net, C++ to match the ILAsm static signature and constructor description - "Provides the static class constructor." Resolves dotnet#2529

MSDN-WhiteKnight reviewed Jun 3, 2019

View reviewed changes

Public static constructor -> static constructor

f9228f3

Per dotnet#2536 (comment) Removed the public access modifier for the C# and VB.Net static constructor. C++ does not support static constructors, so I've reverted the addition of the static modifier.

rpetrusha assigned EugeneKramer Jun 3, 2019

rpetrusha added the blocked Indicates issues or PRs that are blocked for some reason label Jun 3, 2019

rpetrusha added this to the June 2019 milestone Jun 3, 2019

rpetrusha added ✨ 1st-time dotnet-api-docs contributor! Indicates PRs from new contributors to the dotnet-api-docs repository and removed blocked Indicates issues or PRs that are blocked for some reason labels Jun 4, 2019

rpetrusha suggested changes Jun 4, 2019

View reviewed changes

xml/System.Runtime.InteropServices.CustomMarshalers/TypeToTypeInfoMarshaler.xml Outdated Show resolved Hide resolved

xml/System.Runtime.InteropServices.CustomMarshalers/TypeToTypeInfoMarshaler.xml Outdated Show resolved Hide resolved

rpetrusha mentioned this pull request Jun 4, 2019

Static contructor's C# syntax string is missing static #2529

Closed

EugeneKramer and others added 2 commits June 4, 2019 09:27

Update xml/System.Runtime.InteropServices.CustomMarshalers/TypeToType…

4e262dd

…InfoMarshaler.xml Co-Authored-By: Ron Petrusha <[email protected]>

Update xml/System.Runtime.InteropServices.CustomMarshalers/TypeToType…

0d05d50

…InfoMarshaler.xml Co-Authored-By: Ron Petrusha <[email protected]>

rpetrusha approved these changes Jun 4, 2019

View reviewed changes

rpetrusha added the verify-build-before-merge label Jun 4, 2019

Update constructor EnumerableToDispatchMarshaler() signature to publi…

21f240a

…c static for VB.NET & C# Update constructor EnumerableToDispatchMarshaler() signature to public static for VB.NET & C# per dotnet#2536

EugeneKramer added 2 commits June 4, 2019 20:44

Update constructor EnumeratorToEnumVariantMarshaler() signature to pu…

1032c2d

…blic static for VB.NET & C# Update constructor EnumeratorToEnumVariantMarshaler() signature to public static for VB.NET & C# per dotnet#2536

Update constructor ExpandoToDispatchExMarshaler() signature to static…

c070d56

… for VB.NET & C# Update constructor ExpandoToDispatchExMarshaler() signature to public static for VB.NET & C# per dotnet#2536

rpetrusha merged commit 3bbaa66 into dotnet:master Jun 5, 2019

mairaw removed the verify-build-before-merge label Jul 24, 2019

Update constructor TypeToTypeInfoMarshaler() signature to static #2536

Update constructor TypeToTypeInfoMarshaler() signature to static #2536

Uh oh!

Conversation

EugeneKramer commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mairaw Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpetrusha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rpetrusha commented Jun 4, 2019

Uh oh!

rpetrusha commented Jun 4, 2019

Uh oh!

dnfclas commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EugeneKramer commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpetrusha commented Jun 5, 2019

Uh oh!

Uh oh!

EugeneKramer commented Jun 3, 2019 •

edited

Loading

mairaw Jun 4, 2019 •

edited

Loading

dnfclas commented Jun 5, 2019 •

edited

Loading

EugeneKramer commented Jun 5, 2019 •

edited

Loading